r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

The Miri subtree was changed

cc @rust-lang/miri

There are some holes in the behaviour of some operations, because I did not know how I could efficiently handle them, I'll mark them and add some explanation.

Also,
r? @RalfJung

Please create a PR for the libstd changes on their own so a libs reviewer can review it. Once that is done and synced, you can open a PR against the miri repo with the miri changes.

It's the same for me, I kept the commits for Miri and stdlib separate, so if I have to split it up, it's pretty straightforward.

🤷 I'm also fine either way, it'll just be a bunch of work to port the commits to the Miri repo.

I noticed that the C standard doesn't specify the output ranges for all of its mathematical operations; it just specifies them as:

Functions where C does not give an output range shouldn't get their value clamped, I would say.

I moved the "helpers" used for creating fixed outputs and clamping operations to their defined ranges to helpers.rs. These are now also extended to handle the floating-point operations in foreign_items. Tests in miri/tests/float.rs were changed/added.

Please don't, that file is already too big.^^

If the helpers are only needed in one file, keep them there.
Otherwise, add them in src/tools/miri/src/math.rs.

Reopening.

Functions where C does not give an output range shouldn't get their value clamped, I would say.

You're right, but this has some consequences. For example, the sin operation doesn't have an explicitly defined fixed output range, yet we guarantee and test this in Miri: rust-lang/miri#4207.

The C standard for the return values of sin is:

Returns
The sin functions return sin x.

The range of sine x is [-1, 1], but is not explicitly defined.

But asin has the following description:

Returns
The asin functions return arcsin x in the interval [−π/2, +π/2] radians.

I think we have 2 approaches:

• We say this is implementation-defined and revert all the clamps that are not explicitly defined
• We implicitly read the mathematically defined output ranges of those operations.

If we run into libm implementations that can't even guarantee to return a value within the math function's output domain, tbh I would consider that downright broken and worthy of a bug report, or strong reason to just always use our libm on that platform. I'd be a bit hesitant to encourage users to think about this via Miri unless we think there is a chance this is actually a problem in practice (do we know of any platforms where this has been a problem?).

If it's mostly a concern about following the specification to the letter, I think it might be worth a defect report to see if the C committee is willing to codeify the output domain.

I think we have 2 approaches:

I would suggest a third:

• We do this kind of clamping wherever C mandates it, plus wherever it is easy and/or useful. So, fine to do it for sin and cos, but not for the gamma function ;)

But asin has the following description:

The arcsin function likely states this not as a means of guaranteeing precision, but because the inverse sine of x is under-defined -- infinitely many different inputs map to x (for x in [-1, 1]). By giving the interval, the answer becomes uniquely defined.

I emailed a possible defect report to some of the (I think?) relevant contacts at https://www.open-std.org/jtc1/sc22/wg14/www/contacts and copied you both.

@RalfJung If I want to fix that conflict I should rebase with --keep-base right?

@RalfJung If I want to fix that conflict I should rebase with --keep-base right?

You need to fetch the latest master then rebase without --keep-base if you get a conflict. There are never any conflicts if you rebase on top of the same master you branched from, sounds nice :)

Ah yeah, right, I mixed this with something else I did wrong in another pr. Thanks!

The last 3 commits are what changed since your review, Ralf.

@tgross35 could you take a look at the library side?

The test changes LGTM 👍

@rustbot author

Reminder, once the PR becomes ready for a review, use @rustbot ready.

That looks good. :) Please clean up the history, then this is good to go.

What do you consider clean here? I guess that you don't want everything squashed to 1 commit. Would you like the changes to miri in 1 commit and changes to libstd in another one?

Ideally there's three commits:

• one that just moves the code around in Miri unchanged
• one that adds the new logic
• and one that updates libstd

I'll try :).

I think I did this correctly.

@rustbot ready.

You did unfortunately rebase over the latest master, that makes my life a bit harder now. :/ I thought I didn't have to repeat every single time to please use --keep-base...

Looking good though, thanks!
@bors r+

📌 Commit 8978073 has been approved by RalfJung

It is now in the queue for this repository.

🤦 fuck, yeah, I'm sorry...